Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix avif odd width/height handling #2130

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

toppa102
Copy link

I license past and future contributions under the dual MIT/Apache-2.0 license,
allowing licensees to choose either at their option.

Related issue

This PR fixes the InvalidValue error in dcv_color_primitives when trying to decode odd width/height AVIF files and changes PixelLayout::I422 to todo!() since it's not supported by dcv_color_primitives (source).

It's a bit hacky but dav1d's output buffer's width/stride seems to always be a multiple of 128 (not too sure about this though) so no need to account for that just increase the width provided to the convert_image by 1 if it's odd.

And for the height add a padding row of zeros to the source luma buffer.

And at the end copy only the actual width and height parts of the dst buffer while discarding the padding.

@fintelia
Copy link
Contributor

Given that the dcv-color-primitives doesn't really seem to match our requirements well (based on the number of todo!()'s and copies involved) I wonder if it would be better just to implement the logic ourselves.

Based on that crates documentation, the relevant formula is:

r = 1.164 * (y - 16) + 1.596 * (cr - 128)
g = 1.164 * (y - 16) - 0.813 * (cr - 128) - 0.392 * (cb - 128)
b = 1.164 * (y - 16) + 2.017 * (cb - 128)

@toppa102
Copy link
Author

Given that the dcv-color-primitives doesn't really seem to match our requirements well (based on the number of todo!()'s and copies involved) I wonder if it would be better just to implement the logic ourselves.

It's an option, I was also thinking about adding the yuv422 -> rgba and yuv400 -> rgba algorithms to dcv-color-primitives. I took a look at the crate and they seem to have a lot of the constants and other needed stuff already.

width,
height,
rounded_width,
rounded_height,
&src_format,
Some(strides),
&src_buffers,
&dst_format,
None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be worth seeing whether a non-None value here would enable support for odd sized images

@jinx-eob1
Copy link

Any updates on this issue? Lots of avif images out there with odds dimensions.

@awxkee
Copy link

awxkee commented Oct 18, 2024

Given that the dcv-color-primitives doesn't really seem to match our requirements well (based on the number of todo!()'s and copies involved) I wonder if it would be better just to implement the logic ourselves.

Based on that crates documentation, the relevant formula is:

r = 1.164 * (y - 16) + 1.596 * (cr - 128)
g = 1.164 * (y - 16) - 0.813 * (cr - 128) - 0.392 * (cb - 128)
b = 1.164 * (y - 16) + 2.017 * (cb - 128)

This is correct only for Bt.601 matrix with limited range which is not really common nowadays, since it was replaced by Bt.709 matrix in most cases.
Here you can take conversion for yuv400, yuv420, yuv422 and yuv444. Those methods should be in your requirements and supports any image size combinations, and can be easily adopted for 8+ bit-depth or your specific requirements. Here is defined standard if you want to check it. And its simplified inverse matrix

inverse_matrix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants